fix(report): expose per-case execution metrics - #191
Conversation
| if res.Grading != nil && res.Grading.JudgeSession != nil { | ||
| cr.JudgeDurationMs = res.Grading.JudgeSession.DurationMs | ||
| cr.JudgeInputTokens = res.Grading.JudgeSession.InputTokens | ||
| cr.JudgeOutputTokens = res.Grading.JudgeSession.OutputTokens |
There was a problem hiding this comment.
🟠 Major: Retain metrics from failed agent-judge sessions
When an agent judge runs but its output cannot be parsed or validated, runJudgePhaseWithSpan recovers the available session from SessionResultError for artifact handling, but leaves res.Grading nil. This branch therefore drops the judge duration and token usage for ERROR cases, causing the new judge_tokens and overall_tokens totals to underreport actual execution cost.
Suggestion: preserve the recovered judge session on EvalResult and copy metrics from it here for both successful and failed judge executions. Please also extend the existing judge-error regression test with non-zero duration and token values and assert that they reach CaseResult and the aggregate totals.
Source: Codex review @ 9a18eb6f, with cross-file verification against internal/evaluator/evaluator.go:650-660.
There was a problem hiding this comment.
Thanks for tracing this path. After revisiting the intended metric semantics, I don't think metrics from a failed agent-judge session should be included in these report aggregates.
This report is intended to describe valid evaluation samples, rather than serve as a billing or raw resource-consumption ledger:
- tested-agent metrics describe the Skill/case execution being measured;
- judge metrics are recorded only when the judge produces a parseable, validated grading result;
- if the judge fails because of an API/transport error, timeout, invalid JSON, or response-schema validation error, the case is marked
ERRORand that judge run is an invalid measurement sample.
Including partial metrics from such a failed judge run in judge_tokens and overall_tokens would mix infrastructure/tooling anomalies into the statistics used to compare case execution. The recovered SessionResult should still be retained for artifacts and diagnostics, but not promoted into the evaluation aggregates.
Given that semantic choice, I propose to keep the current data flow, explicitly document that judge/overall totals include successfully validated judge sessions only, and add regression coverage confirming that failed judge-session metrics are excluded. If billing-grade accounting is needed later, it should be exposed as a separate raw usage/telemetry metric instead of overloading benchmark metrics.
zpzjzj
left a comment
zpzjzj
left a comment
There was a problem hiding this comment.
Code review summary
Review scope: codex/fix-report-case-metrics → main at 9a18eb6f
Findings
- Blocker: 0
- Major: 1
- Info: 1
Compatibility assessment
The machine-readable result.json contract remains backward-compatible for normal JSON consumers:
- Existing fields such as
total_tokens,duration_ms,input_tokens, andoutput_tokensare retained. total_tokenspreserves its previous meaning: tested-agent tokens only.- Judge and overall metrics are additive fields.
- Regenerating reports from the existing legacy fixture succeeds and preserves
total_tokens=12500, while derivingoverall_tokens=12500.
There is still a compatibility consideration for consumers that parse derived presentation formats. The HTML embedded DATA object renames duration / total_tokens and per-case duration_ms / duration, while the Markdown table schema also changes. No repository or publicly searchable downstream consumer was found, so this is informational rather than blocking. If these representations are consumed by internal automation, consider retaining the old HTML aliases for one release or explicitly documenting that only result.json is a stable machine-readable contract. Strict JSON schemas using additionalProperties: false should also be checked before rollout because the new additive fields would require a schema update.
Overall assessment
Validation reviewed: all GitHub checks are green; make test and targeted race-enabled report, runner, and CLI tests pass.
2 participants
Summary
total_tokensJSON field for compatibilityRoot cause
The top-level report duration comes from
EndTime - StartTime, while each case duration comes from the tested-agent session. The HTML report labeled both scopes ambiguously and omitted the per-case token fields that were already present inCaseResult.Agent-judge sessions also exposed duration and token usage internally, but those metrics were not copied into the report model before
JudgeSessionwas excluded from JSON serialization.This change is engine-independent. Agent-specific token acquisition, including QoderCLI session parsing, is intentionally out of scope.
User impact
Reports now make Skill execution cost visible without allowing metrics to dominate the response and grading content:
Validation
make fmtmake verifygo test -race ./...expression-calculatorusing QoderCLI:Closes #190